Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: TLSv1.3 support #146

Merged
merged 18 commits into from
Mar 17, 2022
Merged

Feat: TLSv1.3 support #146

merged 18 commits into from
Mar 17, 2022

Conversation

kares
Copy link
Contributor

@kares kares commented Nov 24, 2021

Implements TLS 1.3 support in the plugin, using existing tls_min_version / tls_max_version options.
The plugin was lacking TLS testing thus it's been added.

NOTE: a follow-up minor release is planned, where the protocol configuration will be changed to ssl_supported_protocols => ... and the tls_min_version / tls_max_version options are going to be deprecated.
But for now this is worth shipping as a patch release.

@kares kares marked this pull request as ready for review March 16, 2022 11:37
@kares kares mentioned this pull request Mar 16, 2022
40 tasks
@kares kares requested a review from robbavey March 16, 2022 17:58
Copy link
Contributor

@robbavey robbavey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff! Thanks for adding the SSL testing ❤️

Tests are failing on 6.8 with

Java::JavaxNetSsl::SSLHandshakeException:
       No appropriate protocol (protocol is disabled or cipher suites are inappropriate)

when using a modern Java8 JVM that supports TLS1.3, such as OpenJDK Runtime Environment (Temurin)(build 1.8.0_322-b06) on my local box, plus 1.8.0_282-b08 in the testing environment.

spec/fixtures/certs/generate.sh Outdated Show resolved Hide resolved
spec/fixtures/certs/generate.sh Outdated Show resolved Hide resolved
@kares
Copy link
Contributor Author

kares commented Mar 17, 2022

when using a modern Java8 JVM that supports TLS1.3, such as OpenJDK Runtime Environment (Temurin)(build 1.8.0_322-b06) on my local box, plus 1.8.0_282-b08 in the testing environment.

my bad - did not realize the TLS 1.3 detection won't do, due Manticore itself (used for testing) on Java 8 without the jdk.tls.client.protocols=TLSv1.3 system property - simply due the way TLSv1.3 support was added in Java 8

added a different way to detect whether TLS in client mode includes 1.3

@kares kares requested a review from robbavey March 17, 2022 12:16
Copy link
Contributor

@robbavey robbavey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff!

Only thought here is whether this should be a minor update, rather than patch, as it is adding a new feature plus a tiny comment nit

CHANGELOG.md Outdated
@@ -1,3 +1,6 @@
## 3.4.6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this is an added feature, rather than a bug fix, does it make sense to make this a minor rather than patch release?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine by me, thought the changes are fully backwards compatible but it makes sense ... updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants